-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(dashboard): scheduled digest #7314
Conversation
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for dashboard-v2-novu-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -16,7 +16,7 @@ | |||
rel="stylesheet" | |||
/> | |||
<link | |||
href="https://fonts.googleapis.com/css2?family=Ubuntu+Mono:ital,wght@0,400;0,700;1,400;1,700&display=swap" | |||
href="https://fonts.googleapis.com/css2?family=JetBrains+Mono:ital,wght@0,400;0,700;1,400;1,700&display=swap" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the font used for the amount
input
@@ -69,11 +70,13 @@ | |||
"class-variance-authority": "^0.7.0", | |||
"clsx": "^2.1.1", | |||
"cmdk": "1.0.0", | |||
"cron-parser": "^4.9.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the library used to parse and generate cron expression
shouldUnregister?: boolean; | ||
}; | ||
|
||
const AmountInputContainer = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the changes I made here are: split components into smaller independent pieces so they can be reused without being dependent on the form
@@ -61,7 +61,7 @@ const FormLabel = React.forwardRef< | |||
> | |||
<BsFillInfoCircleFill className="text-foreground-300 -mt-0.5 inline size-3" /> | |||
</TooltipTrigger> | |||
<TooltipContent>{tooltip}</TooltipContent> | |||
<TooltipContent className="max-w-56">{tooltip}</TooltipContent> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjust the max width of the tooltip content so it doesn't look weird
import { Command, CommandGroup, CommandItem, CommandList } from '@/components/primitives/command'; | ||
import { selectTriggerVariants } from '@/components/primitives/select'; | ||
|
||
export const MultiSelect = <T extends string | number>({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dropdown component where the user can pick multiple values.
const { second, month, dayOfMonth, dayOfWeek, hour, minute } = useMemo(() => { | ||
try { | ||
const expression = cronParser.parseExpression(value); | ||
return toUiFields(expression.fields); | ||
} catch (e) { | ||
onError?.(e); | ||
|
||
return { | ||
second: [], | ||
minute: [], | ||
hour: [], | ||
dayOfMonth: [], | ||
month: [], | ||
dayOfWeek: [], | ||
}; | ||
} | ||
}, [value, onError]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parse the cron
value and get the form field values.
const handleValueChange = (fields: Partial<UiCronFields>) => { | ||
const cronFields = toCronFields({ | ||
second, | ||
minute, | ||
hour, | ||
dayOfWeek, | ||
dayOfMonth, | ||
month, | ||
...fields, | ||
}); | ||
|
||
onValueChange(cronParser.fieldsToExpression(cronFields).stringify()); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will generate a new cron
expression when any value is changed on the UI.
}; | ||
|
||
const handlePeriodChange = (period: string) => { | ||
onValueChange(getCronBasedOnPeriod(period as PeriodValues, { second, minute, hour, dayOfWeek, dayOfMonth, month })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the period value changes, we need to generate a new cron
expression but preserve the previously selected user values.
/** | ||
* regular expression to check for valid hour format (01-23) | ||
*/ | ||
export function isValidHour(value: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file utils are used by the TimePicker
component
@@ -0,0 +1,403 @@ | |||
import cronParser, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the utils used by the scheduled digest UI to calculate the period from the cron
and based on the current period value selected (when switching).
There are also utils that convert the cron fields to UI field values because the cron-parser
for *
generates the array with all values, but on the UI, we use an empty array as a sign for *
basically every
day, month, hour etc.
@@ -10,7 +10,7 @@ export const EditWorkflowPage = () => { | |||
<EditWorkflowLayout headerStartItems={<EditorBreadcrumbs />}> | |||
<div className="flex h-full flex-1 flex-nowrap"> | |||
<WorkflowTabs /> | |||
<aside className="text-foreground-950 [&_textarea]:text-neutral-600'; flex h-full w-[300px] max-w-[350px] flex-col border-l [&_input]:text-xs [&_input]:text-neutral-600 [&_label]:text-xs [&_label]:font-medium [&_textarea]:text-xs"> | |||
<aside className="text-foreground-950 [&_textarea]:text-neutral-600'; flex h-full w-[350px] max-w-[350px] flex-col border-l [&_input]:text-xs [&_input]:text-neutral-600 [&_label]:text-xs [&_label]:font-medium [&_textarea]:text-xs"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I had to make the sidebar wider, otherwise the scheduled digest UI doesn't fit and look well.
@novu/client
@novu/headless
@novu/framework
@novu/js
@novu/nest
@novu/nextjs
@novu/node
@novu/notification-center
novu
@novu/providers
@novu/react
@novu/react-native
@novu/shared
@novu/stateless
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work on this 🔥
Left a couple of comments.
Screen.Recording.2024-12-18.at.6.24.43.PM.mov
The date selector is a little buggy for the "February" month. I also think we should hide the 30/31 date for feb and "31" for appropriate months. or just have a disabled UI state. as currently it allows selecting the date but the apply button remains stucked.
setDigestType(value); | ||
if (value === SCHEDULED_DIGEST_TYPE) { | ||
setValue(AMOUNT_KEY, undefined, { shouldDirty: true }); | ||
setValue(UNIT_KEY, undefined, { shouldDirty: true }); | ||
setValue(CRON_KEY, EVERY_MINUTE_CRON, { shouldDirty: true }); | ||
} else { | ||
setValue(AMOUNT_KEY, '', { shouldDirty: true }); | ||
setValue(UNIT_KEY, TimeUnitEnum.SECONDS, { shouldDirty: true }); | ||
setValue(CRON_KEY, undefined, { shouldDirty: true }); | ||
} | ||
await trigger(); | ||
saveForm(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest adding a confirmation popup window when switching the digest tabs.
especially when switching from "schedule" to "regular"
As someone might mis-click it and lose all the progress saved in the scheduled form.
Screen.Recording.2024-12-18.at.6.21.01.PM.mov
@BiswaViraj
I agree. I'll talk to Naveen and Radek about that, and we will do it in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only weird thing UX-wise is that the values are discarded after switching the tabs, e.g. if someone has already some complex scheduled digest setup and accidentally clicks on "regular" he will lose the setup.
@ChmaraX yes I agree, there is a ticket to "store" the form values |
What changed? Why was the change needed?
Scheduled digest:
cron
expressionScreenshots
Screen.Recording.2024-12-17.at.12.43.57.mov
Screen.Recording.2024-12-17.at.12.45.39.mov